Skip to content

NAS backup: support incremental backups of RBD volumes - #14386

Open
weizhouapache wants to merge 30 commits into
apache:mainfrom
weizhouapache:24-incremental-backup-rbd-volume-on-nas
Open

weizhouapache wants to merge 30 commits into
apache:mainfrom
weizhouapache:24-incremental-backup-rbd-volume-on-nas

Conversation

@weizhouapache

@weizhouapache weizhouapache commented Oct 12, 2026 •

Copy link
Copy Markdown
Member

Description

Incremental NAS backups so far relied on QEMU dirty bitmaps / libvirt checkpoints, which only
exist on file-based qcow2 storage. RBD images are raw and have nowhere to persist a bitmap, so
any Instance with a volume on Ceph was forced onto the full-only path and re-uploaded the whole
image on every cycle.

Ceph provides the equivalent natively, and this PR adds a second chain format that uses it: an
rbd snap per cycle as the anchor, plus rbd export-diff --from-snap, which computes the
changed extents inside Ceph from the image object-map.

NASBackupProvider now picks the mechanism per Instance in getIncrementalFlavor():

Primary storage Flavor Mechanism
NFS / SharedMountPoint / host-scoped QCOW2 QEMU dirty bitmap + libvirt checkpoint (unchanged)
Ceph RBD RBD rbd snap anchor + rbd export-diff --from-snap
anything else, or a mix of the two UNSUPPORTED legacy full-only

A mix falls back to full-only because the two chain formats cannot be combined inside one backup.
Everything else — chain identity, the nas.backup.full.every cadence, delete/tombstone semantics
and resource accounting — is shared unchanged.

Restore differs from the qcow2 path. An export-diff stream carries no pointer to the diff
before it, unlike a rebased qcow2, so the provider sends the whole chain from its full anchor up
to the backup being restored and the agent replays each diff with rbd import-diff in order. The
image is recreated first: a full diff only carries the extents that were allocated when it was
taken, so any block still allocated in the target would otherwise show through as stale data.
That also drops RBD snapshots the volume carried, which is inherent to rewinding a volume
wholesale. The restore fails closed: if the snapshots of an RBD image can not be listed it
stops, so it never removes snapshots it could not see. An image which does not exist (exit code 2)
has none to see.

Two behaviours follow from the anchor living in Ceph rather than in the QEMU process:

  • a stopped Instance keeps its incremental cadence, since no live QEMU is needed — the opposite
    of the qcow2 path, where a stop forces the next backup to be a FULL;
  • a chain survives an Instance restart or a host migration.

Only the newest anchor is kept per image, and a FULL additionally sweeps every older snapshot
carrying the csbackup- prefix, so a deleted chain, an unassigned offering or a restore cannot
leak snapshots into the cluster.

When the offering is removed, or the last backup of a chain is deleted, the anchors are removed
from a host which has the rbd command, and none is sent when no host has it.

Releasing the guest. While the snapshots of an Instance are taken, its filesystems are frozen
or, if there are several disks and no freeze, the Instance is paused. nasbackup.sh bounds the
freeze, pause and snapshot steps with timeouts and records what it holds in
/run/csbackup/<instance>.guest. The guest is released by

  • a TERM/HUP/INT trap of the script, for a script the agent signals;
  • the take-backup wrapper, when the script failed or was killed because the backup timed out;
  • the agent, for every state file left when it starts, which covers SIGKILL (no trap runs), a
    crash and a restart of the agent.

A guest paused by an operator is not resumed, as the state file says what the script itself held.

One RBD backup per Instance at a time. nasbackup.sh holds a per-Instance flock
(/run/csbackup/<instance>.lock) during the RBD backup, so two backups of an Instance, such as a
scheduled one started by a management server which just restarted while the old one still runs on
the host, can not freeze the guest and share the state file at once. The refused backup exits with
code 21 and the wrapper leaves the guest to the backup which holds the lock.

The Ceph monitor list of an RBD disk is built with RbdUtils, so a bare IPv6 address is bracketed,
carries its port and survives the parsing of nasbackup.sh.

The feature stays behind the existing nas.backup.incremental.enabled zone setting, which still
defaults to false, so nothing changes for existing deployments until it is switched on.

Note: the agent drives the rbd CLI, so KVM hosts backing up Ceph volumes need ceph-common
installed.

Related: #14667, which encrypts a volume created on a storage pool with an encrypted disk
offering (4.22 base).

Test changes included here

  • test_backup_recovery_nas.py: the blanket RBD skip is replaced by an _is_rbd_storage()
    predicate, so the chain tests now run on Ceph. test_stopped_vm_falls_back_to_full keeps a
    targeted skip because RBD has the opposite behaviour by design, and a new
    test_stopped_rbd_vm_keeps_incremental_cadence asserts that other side.
  • The suite moves to a cloud-init template, whose kernel enumerates hot-attached virtio disks and
    which ships the usual filesystem tools, removing guest-side workarounds the legacy template
    needed.
  • A new test_full_chain_restore_and_create_vm_survive_mid_chain_delete walks a whole chain:
    FULL + 3 incrementals, restore from every backup and create an Instance from every backup, then
    delete the middle incremental and repeat both sweeps over the survivors. It also asserts that an
    incremental is a small fraction of its FULL, which is what distinguishes a real export-diff
    delta from the whole image being re-exported.
  • Instances created from a backup are verified by their guest contents over SSH, not only by
    reaching Running.
  • Marvin gives every Instance it creates its own public IP for SSH, and destroying the Instance
    does not release it, so a class which builds many Instances from backups ran out of the public
    range of a small zone. release_publicIP() is added to marvin.lib.common and the tests release
    the IP of every Instance they create when they are done with it. The Instance shared by the class
    keeps its IP until the class ends.

Types of changes

  • Breaking change (fix or feature that would cause existing functionality to change)
  • New feature (non-breaking change which adds functionality)
  • Bug fix (non-breaking change which fixes an issue)
  • Enhancement (improves an existing feature and functionality)
  • Cleanup (Code refactoring and cleanup, that may add test cases)
  • Build/CI
  • Test (unit or integration test code)

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

  • Major
  • Minor

Bug Severity

  • BLOCKER
  • Critical
  • Major
  • Minor
  • Trivial

Screenshots (if appropriate):

How Has This Been Tested?

Tested on two KVM + Ceph/RBD lab environments (zone-wide RBD primary storage with two
Ceph pools, NAS backup repository on an NFS export, nas.backup.incremental.enabled=true).

Unit tests

  • NASBackupProviderTest — 60/60 pass
  • LibvirtRestoreBackupCommandWrapperTest — 30/30 pass
  • LibvirtTakeBackupCommandWrapperTest — 23/23 pass
  • RbdBackupGuestManagerTest — 3/3 pass
  • KVMPhysicalDiskTest — 5/5 pass

Smoke tests — test/integration/smoke/test_backup_recovery_nas.py

The whole suite was run repeatedly on Ceph/RBD to check for flakiness, each run against a
freshly cleaned environment:

Run Scope Result
1 full suite (12 tests) Ran 12 tests in 2471.793s — OK (SKIP=2)
2 11 tests Ran 11 tests in 1398.364s — OK (SKIP=2)
3 11 tests Ran 11 tests in 1356.676s — OK (SKIP=2)
4 11 tests Ran 11 tests in 1369.889s — OK (SKIP=2)
5 chain test only Ran 1 test in 1668.097s — OK
6 chain test only Ran 1 test in 1682.074s — OK
7 full suite (12 tests), RBD-only storage Ran 12 tests in 3207.053s — OK (SKIP=2)
8 full suite (12 tests), RBD-only storage, public IPs released by the tests Ran 12 tests in 3470.479s — OK (SKIP=2)

No failures in any of these runs; failed_plus_exceptions.txt was empty every time. Runs 7 and 8
were made with the same changes on a build based on 4.23.0, after the guest release and the lock
were added; the unit tests above were run on this branch. In between, a run on a zone with only 13
free public IPs failed in the two tests which build an Instance from a backup, because every such
Instance held a public IP until the class ended; that is what the IP release above fixes. Runs 2–4 produced
identical per-test results, and runs 5–6 produced identical guest contents across all 14
verification points, so the new assertions are stable rather than lucky.

All of these pass on Ceph/RBD, where the chain tests previously could not run at all:

  • test_vm_backup_lifecycle
  • test_vm_backup_create_vm_from_backup
  • test_incremental_chain_cadence
  • test_incremental_after_vm_restart
  • test_restore_from_incremental
  • test_delete_middle_incremental_repairs_chain
  • test_delete_full_with_children_is_deferred
  • test_restore_volume_and_attach_to_vm
  • test_stopped_rbd_vm_keeps_incremental_cadence
  • test_full_chain_restore_and_create_vm_survive_mid_chain_delete

The two skips are deliberate, not gaps:

  • test_stopped_vm_falls_back_to_full — skipped on RBD, which has the opposite correct
    behaviour. A qcow2 chain is anchored on a QEMU dirty bitmap that dies with the process, so a
    stopped instance must fall back to a FULL; an RBD chain is anchored on an rbd snap inside
    Ceph, so it keeps its cadence. The RBD side is asserted by
    test_stopped_rbd_vm_keeps_incremental_cadence, which takes an INCREMENTAL while the
    instance is stopped and then restores data written before the stop.
  • test_vm_backup_create_vm_from_backup_in_another_zone — single-zone environment.

On-NAS layout and backup sizes

A FULL + 3 INCREMENTALs of an 8 GiB RBD root volume, taken manually through the API:

Backup Repository directory .rbddiff bytes Size reported by CloudStack
FULL 2026.10.11.14.20.44 1,868,960,404 1,868,968,208 (1.74 GiB)
INCREMENTAL 1 2026.10.11.14.20.56 2,186,206 2,194,010 (2.09 MiB)
INCREMENTAL 2 2026.10.11.14.21.00 1,806,593 1,814,397 (1.73 MiB)
INCREMENTAL 3 2026.10.11.14.21.06 4,442,089 4,449,893 (4.24 MiB)

Each backup directory holds the diff plus the usual metadata:

<repository>/i-2-598-VM/2026.10.11.14.20.44/
    domain-config.xml                                        6989
    domblklist.xml                                            204
    domiflist.xml                                             178
    dominfo.xml                                               433
    root.ae7052e5-3244-48e9-a61c-bc6e0a14ebb0.rbddiff  1868960404

The size CloudStack records is the whole directory: .rbddiff + 7,804 bytes of XML metadata,
matching to the byte in every row above. Incrementals are 0.10 %–0.24 % of the FULL, i.e.
rbd export-diff really is shipping only the changed extents rather than re-exporting the image.

The same ratio holds over a longer run on the second environment — 24 backups taken by the
smoke suite reported FULLs of 2.18–2.25 GB against incrementals of 12.55 KB–20.82 MB, with no
backup reporting a size of 0, and every BackupAnswer carrying bitmapCreated=csbackup-<ts>
and incrementalFallback=false (no silent fallback to a full).

Restore and create-instance-from-backup, for every backup in a chain

With the FULL + 3 INCREMENTAL chain above:

  • in-place restore from each of the 4 backups — all succeeded;
  • create-instance-from-backup from each of the 4 backups — all 4 instances reached Running.

The automated chain test does the same thing and additionally verifies the guest: a marker file
is written before each backup, and after every restore the markers taken up to that backup must
be present and markers written after it must be absent — so a restore that silently did
nothing, or that came up with the source instance's current disk, fails instead of passing. The
instances created from backups are checked the same way over SSH, not merely for reaching
Running. Observed guest contents (7 restores and 7 created instances, each on its own IP):

restores (source instance)   [m0] → [m0,m1] → [m0,m1,m2] → [m0,m1,m2,m3]
created instances            [m0]   [m0,m1]   [m0,m1,m2]   [m0,m1,m2,m3]

Deleting a backup from the middle of the chain

An rbd export-diff stream carries no pointer to the diff before it, so a restore replays the
whole sequence from the full anchor and a diff whose descendant still needs it cannot simply be
dropped. Deleting INCREMENTAL 2 from the chain above:

  • it disappeared from listBackups, while its 1.8 MB diff stayed on the repository because
    INCREMENTAL 3 still needs it, and no other backup's reported size changed;
  • restoring from each of the 3 survivors still succeeded, including INCREMENTAL 3 across the
    deleted parent
    , which came back with all four markers — m2 included, the marker whose
    backup had been deleted;
  • creating an instance from each of the 3 survivors also succeeded, and the instance built from
    INCREMENTAL 3 likewise came up carrying m2, so a freshly built volume replays across the gap
    too, not only an in-place restore.

Chain teardown leaves nothing behind

Deleting the chain afterwards swept the repository completely:

delete INCREMENTAL 3  → 2 directories removed (its own + the tombstoned INCREMENTAL 2)
delete INCREMENTAL 1  → 1 directory remaining
delete FULL           → 0 directories remaining

Deleting INCREMENTAL 3 removed two directories at once: its own, plus the INCREMENTAL 2 that had
been retained only for its sake, swept the moment it became unreferenced. After the manual chain
and six full smoke runs the instance's repository directory was empty and both Ceph pools held
0 csbackup-* snapshots across all images, so no chain leaked a snapshot into the cluster.

How did you try to break this feature and the system with this change?

  • Deleted a backup from the middle of a live chain and then restored and built instances from
    every survivor, including the tip whose parent had been deleted — the case that breaks if the
    provider ever sends a chain with a hole in it.
  • Deleted a FULL that still had children — it is tombstoned and hidden from the list while a
    descendant survives, and physically swept once the last descendant is deleted
    (test_delete_full_with_children_is_deferred).
  • Restored the same instance repeatedly, from older and newer backups in turn, checking after
    each restore that markers written after that backup were rolled back rather than left behind.
  • Stopped the instance and kept backing up — on RBD the cadence must hold instead of falling
    back to a FULL, and the resulting incremental must still restore data written before the stop.
  • Restarted the instance mid-chain — an incremental taken after the restart must still be an
    INCREMENTAL and must restore correctly (test_incremental_after_vm_restart, PR feat(backup): incremental NAS backup support for KVM #13074
    regression).
  • Checked the reported sizes against the repository, since an incremental that silently
    re-exported the whole image would still "work". The byte-for-byte agreement above, and the
    assertion that an incremental is a small fraction of its FULL, are what make that visible.
  • Checked for leaks after every run — repository directories and csbackup-* snapshots in
    both Ceph pools.
  • Released a held guest after the backup script was signalled or killed. The trap on
    TERM/HUP/INT and the per-Instance lock were exercised with a stand-in virsh; the wrapper (a
    timed-out script, a guest paused by an operator, a refused second backup) and the agent start-up
    sweep are covered by unit tests, and the sweep was also run once on a KVM host. A freeze or
    pause interrupted on a real guest by a restart of the agent in the middle of a backup has not
    been tested end to end.
  • Mixed-storage instances (volumes on both RBD and file-based storage) are rejected by
    getIncrementalFlavor() and stay on the legacy full-only path, since the two chain formats
    cannot be mixed within one backup.

test_backup_recovery_nas.py only allowed NFS primary storage, since it
reused the primary storage pool's own path as the NAS backup repository
address, and always required incremental-backup semantics that only
qcow2/NFS storage can provide. Neither holds on Ceph/RBD.

- setUpClass now accepts RBD alongside NFS as the primary storage pool
  type, picking a pool that's actually Up rather than list()[0] --
  environments that added Ceph/RBD after the zone's original NFS
  primary storage keep that old pool around in Disabled state, and it
  still sorts first, silently exercising its path as if it were the
  storage VMs actually deploy on.
- The NAS backup repository's NFS export address is resolved
  independently of the primary storage: when the primary pool isn't NFS,
  reuse the nfs test data entry (services[nfs][url]) -- the same
  temporary NFS mount point test_primary_storage.py uses for its
  temporary NFS primary storage pool, and something every marvin
  environment already has configured. An explicit
  nas_backup_repository_address test data entry or
  NAS_BACKUP_REPO_ADDRESS environment variable, if set, takes precedence.
- The external offering imported in setUpClass is matched to the
  repository just created by externalid (== the repository's own id
  for the nas provider) rather than blindly taking index 0 -- a stray
  repository left over from an earlier interrupted run, whose backups
  didn't get cleaned up so its own teardown couldn't remove it either,
  sorts alongside the new one with no guarantee of which comes first.
- Incremental NAS backups require QEMU dirty bitmaps / libvirt
  checkpoints, which only exist on file-based qcow2 storage
  (NASBackupProvider.allVolumesOnCheckpointCapableStorage). The six
  incremental-chain tests now skip on RBD/Ceph, where the provider
  always falls back to full-only backups server-side, rather than
  failing on assertions that storage type can never satisfy.
- Added test_restore_volume_and_attach_to_vm, which exercises
  restoreVolumeFromBackupAndAttachToVM end-to-end (restoring a backed-up
  ROOT and DATADISK volume onto a second, stopped Instance) -- the API
  that drives the restore-and-attach code fixed by the previous commit
  (apache#14007). The target Instance is stopped with
  forced=True: a graceful ACPI stop was observed to time out (~2
  minutes) before falling back to a hard destroy anyway, and once
  forced to a hard destroy the domain drops out of libvirt entirely,
  so the periodic ping-based PowerState sync the restore call depends
  on falls back to a much slower heuristic well past any reasonable
  wait. A forced stop destroys the domain immediately and
  deterministically.
Incremental NAS backups so far relied on QEMU dirty bitmaps / libvirt
checkpoints, which only exist on file-based qcow2 storage. RBD images are
raw and have nowhere to persist a bitmap, so any VM with a volume on Ceph
was forced onto the full-only path and re-uploaded the whole image on every
cycle.

Ceph provides the equivalent natively, and this adds a second chain format
that uses it: an `rbd snap` per cycle as the anchor plus
`rbd export-diff --from-snap`, which computes the changed extents inside
Ceph from the image object-map. NASBackupProvider now selects the mechanism
per VM via getIncrementalFlavor() - QCOW2 for checkpoint-capable pools, RBD
for Ceph, UNSUPPORTED (legacy full-only) for mixed or other storage, since
the two formats cannot be mixed inside one backup. Everything else - chain
identity, the nas.backup.full.every cadence, delete/tombstone semantics and
resource accounting - is shared unchanged.

Restore differs because an export-diff stream carries no pointer to the diff
before it, unlike a rebased qcow2. The provider therefore sends the whole
chain from its full anchor up to the backup being restored, and the agent
recreates the image and replays each diff with `rbd import-diff` in order.
The image has to be recreated: a full diff only carries the extents that
were allocated when it was taken, so any block still allocated in the target
would otherwise show through as stale data. That also drops RBD snapshots
the volume carried, which is inherent to rewinding a volume wholesale.

Two behaviours follow from the anchor living in Ceph rather than in the QEMU
process: a stopped VM keeps its incremental cadence (no live QEMU needed),
and a chain survives a VM restart or host migration. Only the newest anchor
is kept per image; a full additionally sweeps every older snapshot carrying
the csbackup- prefix, so a deleted chain, an unassigned offering or a
restore cannot leak snapshots.

The feature stays behind the existing nas.backup.incremental.enabled zone
setting, which still defaults to false.

Note: the agent drives the `rbd` CLI, so KVM hosts backing up Ceph volumes
need ceph-common installed.

Verified on a Ceph Reef 18.2.8 cluster with KVM hosts on Ceph Quincy
17.2.7 clients: FULL 1.86 GB followed by incrementals of 71 KB - 2.6 MB,
restores replaying 2- and 3-diff chains across two RBD pools including
through a tombstoned ancestor.

test_backup_recovery_nas.py: the blanket RBD skip is replaced by an
_is_rbd_storage() predicate, so the five chain tests now run on Ceph.
test_stopped_vm_falls_back_to_full keeps a targeted skip because RBD has the
opposite behaviour by design, and a new
test_stopped_rbd_vm_keeps_incremental_cadence asserts that other side.
The suite registered the legacy CentOS 5.5 tiny template via get_template().
Its kernel/udev does not reliably enumerate hot-attached virtio disks and it
ships no mkfs.ext4, so the chain tests needed guest-side workarounds to reach
the data they back up and restore.

Register the Ubuntu 24.04 cloud-init template instead, which enumerates
hot-attached disks, carries the usual filesystem tools and grows its root
filesystem on first boot by itself.

The cloud image disables SSH password authentication outright
(60-cloudimg-settings.conf), so guest access moves to an SSH keypair and the
stock "ubuntu" user. That user lands unprivileged, so commands touching /root
or a raw block device now go through a _sudo() helper which wraps the whole
command in `sh -c` under sudo, keeping redirects and ';'-separated commands
root-privileged rather than just the first word.

Cleanup ordering changes with it: a template cannot be deleted while a VM
still uses it, so it is appended to _cleanup after the account that owns the
VM, since cleanup_resources() deletes in list order.

The in-place multi-disk restore regression test that accompanied this change
upstream is left out: it asserts the uuid-based volume mapping in
LibvirtRestoreBackupCommandWrapper, which this branch does not carry.
The existing chain tests each check one property of one backup: the full/incremental
cadence, a restore from the tip, a delete that repairs or defers. None of them walks
the chain as a whole, and none asserts on backup size.

Add a test that builds FULL + 3 INCREMENTALs with a marker file per backup, restores
from every backup and creates a VM from every backup, then deletes the 2nd incremental
and repeats both sweeps over what survives.

The mid-chain delete is what earns the runtime on RBD. An `rbd export-diff` stream
carries no pointer to the diff before it, so a restore replays the whole sequence from
the full anchor, and a diff whose descendant still needs it cannot be dropped when it
is deleted, only hidden. Restoring the tip across that gap is what breaks if the
provider ever sends a chain with a hole in it, and nothing else covered it.

Sizes are asserted throughout: an incremental must be a small fraction of the full.
That is what distinguishes a real export-diff delta from the whole image being
re-exported every cycle, and no other test in this file looks at size at all, so a
provider that silently fell back to full-every-time still passed.

Each restore also asserts that markers written after that backup are gone. Checking
only that the expected markers are present would pass even if the restore had quietly
done nothing.

VMs built from backups are expunged as the test goes rather than through _cleanup:
seven of them are created, and holding them all to the end risks exhausting the
environment's capacity partway through a run.
…t boots

The chain test checked an Instance created from a backup only for reaching
Running. That proves the image boots; it says nothing about whether the diff
chain was replayed into the new volume correctly, so a create-from-backup that
produced a stale or empty image would still have passed.

Check the guest the same way an in-place restore is checked: the markers taken
up to that backup must be present, and markers written after it must be absent.
The absent half matters as much as the present half -- without it, a VM that
somehow came up with the source Instance's current disk would pass.

Access uses the pattern already proven by test_incremental_after_vm_restart: the
backup carries the source Instance's authorized_keys, so the new VM accepts the
same keypair and the stock cloud-image "ubuntu" user.
@weizhouapache

Copy link
Copy Markdown
Member Author

Note: this changes the same file with #14194 #14203 #14374

@weizhouapache

Copy link
Copy Markdown
Member Author

@blueorangutan package

@weizhouapache weizhouapache added this to the 24.0 milestone Oct 12, 2026
@codecov

codecov Bot commented Oct 12, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 3.87%. Comparing base (82e5ede) to head (7473aa1).
⚠️ Report is 33 commits behind head on main.

❗ There is a different number of reports uploaded between BASE (82e5ede) and HEAD (7473aa1). Click for more details.

HEAD has 1 upload less than BASE
Flag BASE (82e5ede) HEAD (7473aa1)
unittests 1 0
Additional details and impacted files
@@              Coverage Diff              @@
##               main   #14386       +/-   ##
=============================================
- Coverage     20.68%    3.87%   -16.81%     
=============================================
  Files          6489      493     -5996     
  Lines        590173    42609   -547564     
  Branches      72362     8076    -64286     
=============================================
- Hits         122060     1653   -120407     
+ Misses       454842    40725   -414117     
+ Partials      13271      231    -13040     
Flag Coverage Δ
uitests 3.87% <ø> (+0.13%) ⬆️
unittests ?

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@abh1sar
abh1sar self-requested a review October 12, 2026 09:57
@weizhouapache

Copy link
Copy Markdown
Member Author

@blueorangutan package

@blueorangutan

Copy link
Copy Markdown

@weizhouapache a [SL] Jenkins job has been kicked to build packages. It will be bundled with no SystemVM templates. I'll keep you posted as I make progress.

@blueorangutan

Copy link
Copy Markdown

Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 19521

rp- added a commit to LINBIT/cloudstack that referenced this pull request Oct 13, 2026
The content-based path overloaded ChainDecision.mode with content-full and
content-incremental, and apache#14386 is adding rbd-full and rbd-incremental the same
way. Split the two concerns as suggested in review: mode stays full or
incremental, and a new method field says how the backup is taken -- qcow2 (push
mode with dirty bitmaps) or content (pull mode). Further methods such as an RBD
diff can be added without new mode tokens.

TakeBackupCommand gets a method field and nasbackup.sh a --method option. Both
default to qcow2 when absent, so a management server that does not send a method
keeps working against an upgraded agent. legacy-full is unchanged: rewriting it
as full plus a "none" method would break incremental-disabled backups on agents
that have not been upgraded yet.
The RBD path overloaded ChainDecision.mode with rbd-full and rbd-incremental,
the same way the content-based path upstream overloaded it with content-full
and content-incremental. Split the two concerns as that path already did: mode
stays full or incremental, and a new method field says how the backup is taken
-- qcow2 (push mode with dirty bitmaps) or rbd (Ceph-native, anchored on an rbd
snapshot and exported with rbd export-diff).

TakeBackupCommand gets a method field and nasbackup.sh a --method option. Both
default to qcow2 when absent, so a management server that does not send a method
keeps working against an upgraded agent. legacy-full is unchanged: rewriting it
as full plus a "none" method would break incremental-disabled backups on agents
that have not been upgraded yet.

CHAIN_FORMAT stays as it is. The method says how a backup is taken and lives
only for the duration of the request; the chain format is persisted and says
what is on the repository, which is what restore and delete read later. The two
coincide for the rbd method today but answer different questions.

This mirrors c144d09 from apache#14194, which
introduced the method for the content path and named an RBD diff as the next
one to adopt it. Kept as close to that commit as the branch allows so the two
merge cleanly: this branch carries the RBD path but not the content one, so the
METHOD_CONTENT token is defined without a user here.
@weizhouapache
weizhouapache force-pushed the 24-incremental-backup-rbd-volume-on-nas branch from 5a3728f to a8fddbf Compare October 14, 2026 09:24
@weizhouapache

Copy link
Copy Markdown
Member Author

as suggested by @abh1sar 's comment: #14194 (comment)
updated with changes in @rp- 's commit: c144d09

@weizhouapache

Copy link
Copy Markdown
Member Author

@blueorangutan package

@blueorangutan

Copy link
Copy Markdown

@weizhouapache a [SL] Jenkins job has been kicked to build packages. It will be bundled with no SystemVM templates. I'll keep you posted as I make progress.

@wido wido left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I went through this PR manually first and then used Claude Fable to check it further. Overall it looks good. The approach is the right one for Ceph: an rbd snapshot as the anchor and export-diff/import-diff for the deltas. The chain logic, the format check against the parent and the incomplete-chain guard on restore are all sound.

Checked locally: NASBackupProviderTest (26) and LibvirtRestoreBackupCommandWrapperTest (21) pass. The script's parse_rbd_opts was exercised with strings in the exact shape RBDStringBuilder emits, including bracketed IPv6 monitors with ports and a cephx-off pool, and parses them correctly. Not tested on a real cluster.

One bug in the restore path and one packaging gap, inline. Two things for the docs or release notes: every backed-up image now carries a permanent snapshot, so changes between two cycles stay allocated until the next backup, and a restore drops all RBD snapshots of the volume, including volume snapshots kept on primary.

Comment thread scripts/vm/hypervisor/kvm/nasbackup.sh Resolved
Comment thread scripts/vm/hypervisor/kvm/nasbackup.sh Resolved
@blueorangutan

Copy link
Copy Markdown

Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 19548

@weizhouapache

Copy link
Copy Markdown
Member Author

thanks @wido !
I will address your comments

Everything that talks to Ceph needs the pool's monitors as a list the client
can parse, and the rules are the same wherever that list is built: give each
monitor the pool's port unless it names one already, and put a bare IPv6
address in square brackets so its own colons stay distinguishable from the
separator before the port.

The mon_host option of a disk's rbd: string wants the same list again, with
':' and ';' escaped, because those two separate the options of that string and
its monitors.

None of it depends on a storage pool, so collect it in RbdUtils as static
methods with their unit tests, rather than leaving each caller to grow a copy
that is right in a different set of cases.
@blueorangutan

Copy link
Copy Markdown

@weizhouapache a [SL] Jenkins job has been kicked to build packages. It will be bundled with no SystemVM templates. I'll keep you posted as I make progress.

@blueorangutan

Copy link
Copy Markdown

Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 19769

@weizhouapache

Copy link
Copy Markdown
Member Author

@abh1sar
thanks for pointing out the errors. changes have been made.
I will run manual test and smoke test with 2 ceph storage as well

@weizhouapache

Copy link
Copy Markdown
Member Author

@abh1sar thanks for pointing out the errors. changes have been made. I will run manual test and smoke test with 2 ceph storage as well

smoke test looks good

Passed (10):

test_delete_full_with_children_is_deferred (44s)
test_delete_middle_incremental_repairs_chain (237s)
test_full_chain_restore_and_create_vm_survive_mid_chain_delete (2280s). This restores every backup in a full-plus-3-incrementals chain and creates a VM from each, which is why it was slow.
test_incremental_after_vm_restart (247s)
test_incremental_chain_cadence (118s)
test_restore_from_incremental (218s)
test_restore_volume_and_attach_to_vm (274s)
test_stopped_rbd_vm_keeps_incremental_cadence (153s)
test_vm_backup_create_vm_from_backup (172s)
test_vm_backup_lifecycle

Skipped (2):

test_stopped_vm_falls_back_to_full is meant for non-RBD storage; its RBD counterpart passed.
test_vm_backup_create_vm_from_backup_in_another_zone needs two zones, and n153 has one.

@blueorangutan

Copy link
Copy Markdown

[SF] Trillian test result (tid-150)
Environment: kvm-ubuntu26 (x2), zone: Advanced Networking with Mgmt server u26, storage: Ceph (x2)
Total time taken: 78132 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr14386-n150-kvm-ubuntu26.zip
Smoke tests completed. 153 look OK, 4 have errors, 0 did not run
Only failed and skipped tests results shown below:

Test Result Time (s) Test File
test_01_migrate_VM_and_root_volume Error 77.85 test_vm_life_cycle.py
test_02_migrate_VM_with_two_data_disks Error 28.41 test_vm_life_cycle.py
test_01_create_vm_snapshots Error 156.95 test_vm_snapshots.py
test_02_revert_vm_snapshots Failure 90.24 test_vm_snapshots.py
test_03_delete_vm_snapshots Failure 0.03 test_vm_snapshots.py
test_01_root_volume_encryption Failure 101.57 test_volumes.py
test_02_data_volume_encryption Failure 232.64 test_volumes.py
test_03_root_and_data_volume_encryption Failure 62.02 test_volumes.py
test_hostha_kvm_host_degraded Failure 640.16 test_hostha_kvm.py
test_hostha_kvm_host_fencing Failure 642.30 test_hostha_kvm.py
test_hostha_kvm_host_recovering Failure 638.93 test_hostha_kvm.py

@abh1sar abh1sar left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

weizhouapache added a commit to weizhouapache/cloudstack that referenced this pull request Oct 29, 2026
@nvazquez
nvazquez marked this pull request as ready for review November 9, 2026 21:25
Copilot AI balanced review requested due to automatic review settings November 9, 2026 21:25

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Unresolved issues can delete user snapshots, leave guests suspended or frozen after timeouts, misroute Ceph operations with IPv6 monitors, and leave snapshots pinning storage when cleanup selects an unsuitable host.

4 open issues
What changed in this PR

Extends CloudStack’s NAS backup provider with Ceph-native incremental backups, complementing the existing qcow2 mechanism while retaining the default-disabled feature setting.

Changes:

  • Adds RBD snapshot anchors, incremental exports, ordered diff replay for restores, and anchor cleanup.
  • Reports host RBD CLI capability and selects suitable hosts, with legacy full-backup fallback when needed.
  • Expands unit and smoke coverage for chain lifecycle, stopped instances, restores, and guest data verification.
File Description
utils/​src/​test/​java/​com/​cloud/​utils/​RbdUtilsTest.java Tests monitor formatting and escaping.
utils/​src/​main/​java/​com/​cloud/​utils/​RbdUtils.java Adds shared RBD monitor-formatting helpers.
test/​integration/​smoke/​test_backup_recovery_nas.py Adds RBD chain and restore scenarios using cloud-init guests.
scripts/​vm/​hypervisor/​kvm/​nasbackup.sh Implements RBD snapshot, export-diff, and cleanup operations.
plugins/​hypervisors/​kvm/​src/​test/​java/​com/​cloud/​hypervisor/​kvm/​resource/​wrapper/​LibvirtRestoreBackupCommandWrapperTest.java Tests diff replay and restore-target safeguards.
plugins/​hypervisors/​kvm/​src/​test/​java/​com/​cloud/​hypervisor/​kvm/​resource/​wrapper/​LibvirtRemoveBackupAnchorsCommandWrapperTest.java Tests anchor-cleanup command handling.
plugins/​hypervisors/​kvm/​src/​main/​java/​com/​cloud/​hypervisor/​kvm/​resource/​wrapper/​LibvirtTakeBackupCommandWrapper.java Validates RBD backup arguments and tolerates non-size output.
plugins/​hypervisors/​kvm/​src/​main/​java/​com/​cloud/​hypervisor/​kvm/​resource/​wrapper/​LibvirtRestoreBackupCommandWrapper.java Recreates RBD images and replays backup diffs.
plugins/​hypervisors/​kvm/​src/​main/​java/​com/​cloud/​hypervisor/​kvm/​resource/​wrapper/​LibvirtRemoveBackupAnchorsCommandWrapper.java Dispatches RBD anchor cleanup to the script.
plugins/​hypervisors/​kvm/​src/​main/​java/​com/​cloud/​hypervisor/​kvm/​resource/​LibvirtComputingResource.java Reports RBD CLI availability.
plugins/​backup/​nas/​src/​test/​java/​org/​apache/​cloudstack/​backup/​NASBackupProviderTest.java Tests RBD decisions, host selection, and cleanup.
plugins/​backup/​nas/​src/​main/​java/​org/​apache/​cloudstack/​backup/​NASBackupProvider.java Coordinates RBD chains, restore paths, and host selection.
plugins/​backup/​nas/​src/​main/​java/​org/​apache/​cloudstack/​backup/​NASBackupChainKeys.java Defines RBD method and chain-format metadata.
packaging/​suse15/​cloud.spec Suggests ceph-common for SUSE agents.
packaging/​el8/​cloud.spec Suggests ceph-common for EL8 agents.
debian/​control Suggests ceph-common for Debian agents.
core/​src/​main/​java/​org/​apache/​cloudstack/​backup/​TakeBackupCommand.java Documents the RBD backup method in a field comment.
core/​src/​main/​java/​org/​apache/​cloudstack/​backup/​RestoreBackupCommand.java Carries backup format and ordered chain directories.
core/​src/​main/​java/​org/​apache/​cloudstack/​backup/​RemoveBackupAnchorsCommand.java Adds the anchor-removal agent command.
api/​src/​main/​java/​com/​cloud/​host/​Host.java Defines the RBD CLI capability key.

Review effort: Balanced

Comment thread scripts/vm/hypervisor/kvm/nasbackup.sh Resolved
Comment thread scripts/vm/hypervisor/kvm/nasbackup.sh Resolved
When the host refuses or fails the restore, the NAS provider returned only false and the backup manager answered with the generic error, so the reason the host gave, such as an RBD image carrying snapshots the backup path did not take, never reached the user. Throw a BackupProviderException with the details of the answer, which the backup manager hands to the user as it is.
…rom a backup

Restoring an RBD volume from a backup rewinds the image wholesale, which removes every snapshot it carries, so the host refuses it when the image carries a snapshot the backup path did not take. Check the volume snapshots kept on primary storage in the management server first, and refuse before anything is sent to the host with the names of the volume and of its snapshots. The host still checks the image, which also catches snapshots CloudStack does not know about.
Copilot AI balanced review requested due to automatic review settings November 10, 2026 13:36

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota. Try requesting a review at a lower effort level.

NASBackupProviderTest imported com.cloud.storage.DataStoreRole twice, which the checkstyle check of the build refuses.
Copilot AI balanced review requested due to automatic review settings November 10, 2026 13:47

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota. Try requesting a review at a lower effort level.

Marvin gives every Instance created by VirtualMachine.create or
Backup.createVMFromBackup its own public IP, with a port forwarding rule
for SSH, and destroying the Instance does not release that IP. The class
only got them back when its account was removed, so the tests which create
Instances from backups ran out of the 13 free addresses of the public range
and failed with "Insufficient address capacity", after which Marvin silently
left ssh_ip unset.

Add release_publicIP() to marvin.lib.common, which releases a public IP
unless it is the source NAT address of the network, and call it for the
Instances of each test in tearDown, for the target Instance of
test_restore_volume_and_attach_to_vm, and for each Instance which
test_full_chain_restore_and_create_vm_survive_mid_chain_delete destroys as
it goes. The Instance shared by the class keeps its IP until the class ends.
Copilot AI balanced review requested due to automatic review settings November 11, 2026 14:39

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota. Try requesting a review at a lower effort level.

The agent assembled the monitor list in several places, each with its own idea
of the rules. Opening a RADOS connection, or writing a ceph.conf for one,
appended the pool's port to the host string as a whole, so with several
monitors only the last was given it and a monitor that already named its port
was handed a second. The rbd: option string of a disk escaped the colons of an
IPv6 address only when it was bracketed already, which left a bare one reading
as though it carried options of its own.

Use the shared helpers for all of them. Two inputs therefore come out
differently, both previously wrong: a monitor that names its port keeps just
that one, and a bare IPv6 address is bracketed before anything is appended to
it or escaped in it.

The iscsiadm portals in IscsiAdmStorageAdaptor read the same way but are not
monitor lists, so they keep their own formatting.
- restore: fail closed when the snapshots of an RBD image can not be listed, so a restore
  never removes snapshots it could not see; a missing image (exit code 2) has none.
- removing the offering sends the anchor cleanup to a host which has the rbd command, and
  sends none when no host has it.
- build the Ceph monitor list of an RBD disk string with RbdUtils, so a bare IPv6 monitor
  is bracketed, carries its port and survives nasbackup.sh's parser.
- RBD backup: bound the freeze, pause and snapshot steps with timeouts and keep a state file
  in /run/csbackup while the guest is held. The guest is released by a TERM/HUP/INT trap of
  the script, by the take-backup wrapper when the script failed or was killed, and by the
  agent for every state file left when it starts (RbdBackupGuestReleaser), which covers
  SIGKILL, a crash and a restart of the agent.
- nasbackup.sh holds a per-VM flock during the RBD backup, so two backups of a VM can not
  freeze the guest and share the state file at once. A refused backup exits with 21 and the
  wrapper then leaves the guest to the backup which holds the lock.
Copilot AI balanced review requested due to automatic review settings November 11, 2026 14:45

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota. Try requesting a review at a lower effort level.

@weizhouapache

Copy link
Copy Markdown
Member Author

@blueorangutan package

@blueorangutan

Copy link
Copy Markdown

@weizhouapache a [SL] Jenkins job has been kicked to build packages. It will be bundled with no SystemVM templates. I'll keep you posted as I make progress.

@blueorangutan

Copy link
Copy Markdown

Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 19993

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants